We can possibly use functools.singledispatch here to consolidate these methods. (as I familiarize myself more with this pattern I realize that's exactly what we should do)

I'm actually confused by the presence of these two types of visit method, really all .visit() is doing here is calling Entry.accept(), which ultimately calls a .visit*() method? (In a sort of triple dispatch back and forth). Maybe all the .visit()'s are meant to be interchangeable?

It looks like I made up .visit without realizing. There's a little more discussion in the below thread.

I'm seeing this elsewhere as starting with an "accept" call, such as root.acept(visitor). These are functionally equivalent right?

You're right, looks like I made up .visit without realizing. It does give us a benefit though: we can use .visit for tracking state during traversal. For example, SearchVisitor adds the Entry to the path and pops it off the path in .visit because .visit doesn't return until all of the reachable entries have been traversed, whereas .visit* return before the reachable entries are traversed. Without .visit, we might have to move the traversal logic into .visit*, so we can track the state we need.

I'll try out removing .visit and seeing what changes need to be made.

For reference, a sequence diagram and class diagram from the source of all my knowledge: https://en.wikipedia.org/wiki/Visitor_pattern#Structure

Because it confused me as I was reading this.

This would be the place to expose any entry that matches, and I think it might be worth bubbling that up and making visit yield as well. I can imagine other visitors gathering PVs or specific entries etc, but not every node we visit will yield a result (Entries that don't match). So in the end maybe only EntryVisitor.visit returns a generator and whatever other worker methods return values as necessary?

In untested skeleton code I'm imagining here (consolidating all the visit_ methods):

    def visit(self, entry: Union[Entry, Root, UUID]) -> None:
        if isinstance(entry, UUID):
            entry = self.backend.get_entry(entry)
        self.path.append(entry)

        # Perform work
        result = self._check_match(entry)
        if result is not None:
            yield result

        # visiting entry's children handled by Entry.accept() calls, previously in super().visit(entry)
        # but we get to visit via an `Entry.accept()` call, so why do we call accept again here?

        self.path.pop()

Excuse my general confusion here

This is basically how I was thinking to use a generator. Per the above thread, if we remove .visit then the generator might become .accept, but the idea is basically the same.

RE consolidating .visit*: In SearchVisitor all of the .visit* methods are identical because we searches treat the different Entry subclasses the same: we check if they match the search terms. Other visitors might treat the subclasses differently, like SnapVisitor only saving data for Parameters. So in general we can't consolidate the .visit* methods, but in some specific visitors they'll be identical.

I agree that the visitors want to differentiate between Entry subclasses, I was arguing more for doing that via single dispatch and overloading the .visit() method.

In that case we won't have to make the Entry.accept methods pick the correct .visit method, the Visitor will get the correct one at runtime